🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18129

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Run pull request jobs on OSDC runners in shadow mode

⚠️ 10 Awaiting Approval, 3 Unrelated Failures

As of commit 3040dc3 with merge base 9ccbc4a ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorchbot label "partner: arm"

@pytorchbot label "release notes: exir"

@pytorchbot label ciflow/trunk

To add these label(s) (ciflow/trunk) to the PR, please first approve the workflows that are awaiting approval (scroll to the bottom of this page).

This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows.

@JacobSzwejbka @lucylq
This is a follow-up to #17333.

• Added a flatbuffer-to-program reader
• Removed the JSON program conversion path

Please let me know if part 2 is too much. I made it a separate commit.

Thanks

@claude review this PR.

Claude finished @digantdesai's task in 2m 24s —— View job

Review of PR #18129

• Read changed files and gather context
• Analyze the flatbuffer-to-program reader implementation
• Review test changes
• Review removal of JSON conversion path
• Post review feedback

This is a well-structured two-commit PR. Commit 1 adds a direct FlatBuffer-to-Program reader; Commit 2 removes the now-redundant JSON/flatc conversion path. The overall direction is sound -- eliminating the JSON+flatc roundtrip removes external tool dependencies, reduces I/O, and avoids large temporary files.

Observations and Suggestions

1. Typo in test_program.py:637

As Copilot noted, "implicity" should be "implicitly":

# this also implicity tests the case where we try parsing the entire

Fix this →

2. _matches_non_dataclass_union_type -- overly broad list matching (_flatbuffer_program.py:256)

if union_origin is list and hasattr(val, "__iter__"):
    return True

This matches any iterable value against any List[...] union arm. If a Union ever contained List[int] alongside List[str], this would always pick the first one regardless of element types. Currently this works because the schema unions don't have competing list types, but it's fragile. Consider adding a comment noting this assumption, or checking element types when the list is non-empty.

3. _matches_non_dataclass_union_type -- str vs bytes coercion (_flatbuffer_program.py:253)

if union_type is str and isinstance(val, (bytes, bytearray, memoryview)):
    return True

This silently matches bytes to str, relying on downstream UTF-8 decoding in _convert_from_flatbuffer_value. If the bytes are not valid UTF-8, the error will surface far from the actual type-matching decision. This is inherent to flatbuffer's representation of strings as byte arrays so it's reasonable, but worth a brief comment.

4. Dead code left behind in _flatbuffer.py

After removing _program_json_to_flatbuffer and _program_flatbuffer_to_json, these items in _flatbuffer.py are no longer used by any Program serialization path:

• _SAVE_FLATC_ENV (line 20) -- only referenced by the deleted function
• _flatc_compile / _flatc_decompile / _run_flatc -- still used by other backends (flat_tensor, etdump, bundled_program, xnnpack, vulkan, qualcomm, etc.)

So _SAVE_FLATC_ENV could be cleaned up, though it's minor. The flatc functions themselves are correctly retained since other serialization paths depend on them.

5. Module-level caches are never cleared (_flatbuffer_program.py:37-41)

_T_CLASS_CACHE: Dict[type, type] = {}
_FIELD_NAME_CACHE: Dict[type, tuple[tuple[str, str], ...]] = {}
_TYPE_HINTS_CACHE: Dict[type, Dict[str, Any]] = {}

These module-level dicts grow monotonically. In the normal ExecuTorch workflow (export once, serialize, done) this is fine. But in long-running processes or test suites that exercise many different schemas, they never shrink. This is not a bug -- just noting it for awareness. The existing _T_CLASS_CACHE and _FIELD_NAME_CACHE from commit 1 have the same characteristic, so this is consistent.

6. _patch_generated_module_aliases mutates module globals (_flatbuffer_program.py:215-221)

This function replaces module-level names (e.g., ExecutionPlan module reference -> ExecutionPlan class) permanently. It's cached via lru_cache so it only runs once, and the comment explains the reasoning well. Just noting this is a global mutation that could interact poorly if any code later needs the module reference rather than the class. The lru_cache(maxsize=1) on _patch_generated_flatbuffer_aliases properly ensures it runs exactly once.

7. _convert_from_flatbuffer_value -- expected_type(val) fallback (_flatbuffer_program.py:309-310)

if isinstance(expected_type, type):
    return expected_type(val)

This is the last-resort conversion at the bottom of _convert_from_flatbuffer_value. It calls the expected type's constructor with val directly. This works for primitives (int, float, str, bool), but if an unexpected type hits this path, the error message may be confusing. The ordered type-checking above should prevent that in practice.

8. _convert_double handles str double_val correctly (_flatbuffer_program.py:140-148)

The special handling for Double where double_val can be a str ("inf"/"-inf") is correctly implemented in both directions. _convert_double checks isinstance(double_val, str) and converts via float(), while the reverse path in _convert_from_flatbuffer_value relies on the type hint Union[float, str] to handle this correctly. The new test test_deserialize_pte_binary_with_rich_flatbuffer_types verifies the inf round-trip at line 532, which is good coverage.

Test Coverage

The test changes are solid:

• test_flatbuffer_program.py is simplified to test only the direct Python roundtrip, which is the only remaining path.
• test_program.py adds test_deserialize_pte_binary_with_rich_flatbuffer_types covering unions (MoveCall, JumpFalseCall, FreeCall), Double("inf"), CompileSpec, delegates, and stacktraces. This exercises the new code paths well.
• All "peek inside flatbuffer" tests in test_program.py are migrated from _json_to_program(_program_flatbuffer_to_json(...)) to the new _flatbuffer_to_program(...).
• The deleted tests in test_flatbuffer.py (TestProgramJsonToFlatbuffer) only tested the now-removed JSON serialization error handling, so their removal is correct.

Summary

The PR is well-organized and the implementation is correct. The main actionable items are:

1. Fix the "implicity" typo (minor)
2. Consider adding a comment about the broad list matching in _matches_non_dataclass_union_type

Everything else is minor or informational. Good work continuing the migration away from the flatc dependency for Program serialization.

@chizkiyahu whats next here?

Workflows were awaiting approval. CI has now been triggered for the ciflow labels on this PR.

@digantdesai
I rebase and fix
is ready to merge